Skip to content

Implement plan validation and ledger-backed review core - #4

Merged
mchwang merged 22 commits into
mainfrom
codex/plan-format-and-linking
Sep 23, 2026
Merged

mchwang merged 22 commits into
mainfrom
codex/plan-format-and-linking

Conversation

@mchwang

@mchwang mchwang commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Problem and result

PR #1 established the design and v1 contract, but the repository had no executable foundation. This adds plan import/validation, pure suggestion transformations, read-only Git history loading, ledger-backed attribution, and approval/choice staleness. It now targets main and incorporates the merged design baseline 91fd2b4.

Implementation

  • Retained versioned schemas and registry-selected v1 semantics, with strict JSON/YAML parsing, decoded duplicate-key detection, byte/depth limits, and version-specific conformance fixtures.
  • Exact full-argv approval and deterministic tokenization. Required selected issue, stable repository/task/plan identity, typed base entries, and caller-supplied actual checkout path identity.
  • Projected leaf/path operations, symlink lineage and declared-target overlap checks; gitlink authoring rejected. Runtime filesystem checks remain separate.
  • Suggestions require the trusted request-time plan binding; Apply increments revision and stales sibling cards. Persistent IDs, replay protection, cancellation and atomic CAS belong to the store.
  • Ledger-only ownership for text and non-text changes, explicit null owners remain foreign, and typed blob/commit metadata in file cards. Approvals and standalone choices bind to stable plan identity.
  • Git reads are bounded and disable inherited Git environment/config, external helpers, lazy fetch, and transports; object alternates and symlinked object storage are rejected. Linear histories only.

Validation

  • Final local npm test: 136 passing tests, including disposable real Git histories.
  • npm run typecheck and git diff --check: pass.
  • Baseline was 60 passing tests. The merged-contract regression suite first reproduced 18 failures; all now pass.
  • Tests cover parsing boundaries, exact argv, registry copies/IDs, projected types and filesystem identity, delayed cross-plan suggestions, approval transfer, typed file metadata, and all prior history regressions.
  • No agent execution, Docker, database, UI, or real-user go/no-go test ran: those phases do not exist in this slice.

Decisions and remaining work

core remains free of runtime I/O. Trusted context comes from the future runner/store, never from model output. The caller must supply actual filesystem identity and keep Git storage stable while reading. The pure suggestion function is not an HTTP endpoint or concurrency boundary.

See docs/implementation/build-step-1.md for API responsibilities and review history. #6 remains open for runtime link/occupancy audits, persistent suggestion identity/CAS, prompt and process budgets, and container enforcement. #2 and #3 remain the next approved build steps. Future typed symlink/gitlink authoring is tracked in #5 and #7.

Review status

The earlier foundation completed nine Copilot rounds; its last round had no new findings. This update changes the contract alignment substantially and requests a fresh review and current-head CI. Prior valid findings were fixed with regressions; declined Git alias/YAML claims and rename-card ownership interpretation are documented in the implementation notes. PR remains unmerged.

A local follow-up regression reproduced acceptance of C1 Unicode controls in paths/commands. The tokenizer and path checks now reject the full Unicode control category; the final suite is 136 passing tests with typecheck and whitespace checks passing.

Merged-contract review round 1

Fixed both stale README examples. Declined the scored-rename report because the regex already captures only the status letter; a new real-Git regression observed and successfully parsed a scored rename followed by another raw record before any parser change. All three threads answered and resolved. Final local suite: 136 passing tests, typecheck and whitespace checks pass. Fresh current-head review requested.

Merged-contract review round 2

This round reviewed earlier commit 9a2df1f. Declined both findings with executable probes: the fingerprint already includes the full shorthand item property (five independent field changes stale approval), and linkTarget calls the canonical path validator for every component (three .git/case/parent spellings reject rename while edit/delete repair passes). No code change was needed. Threads answered/resolved; requesting review of current head a8534cc.

Merged-contract review round 3

Fixed six findings after seven focused fixtures reproduced failures: bounded replay allocation/work and total runtime, shared Git subprocess deadline, metadata lineage through rename/deletion, non-directory link-target traversal, isolated unsafe-link repairs, and reverse-ancestry target overlap. Extended coverage to exact budgets, origin fanout, and text/empty/binary deletion cards. Final local suite: 132 tests, typecheck and whitespace checks pass. All six threads resolved and fresh review requested.

Merged-contract review round 4

Addressed the summary-only metadata reuse finding from review 5287702535. A real binary deletion/recreation regression reproduced lost earlier ownership; updates now merge existing path evidence before storing/propagating it. Owned and foreign deletion cases both pass, preserving Ambiguous/Unplanned classification. Final local suite: 134 tests, typecheck and whitespace checks pass. Fresh review requested.

Merged-contract review round 5

Reproduced the Git graft ancestry bypass using an unrelated root commit. The reader now rejects graft/shallow metadata before ancestry reads and disables grafts in child processes. Common-directory discovery covers linked-worktree metadata. Added graft/shallow regressions and clarified stable-metadata caller responsibility. Final local suite: 136 tests, typecheck and whitespace checks pass. Thread resolved and fresh review requested.

Final verification for the merged-contract update

Current head 611aedd targets main. Six additional Copilot rounds completed; review 5287900922 covers this exact head and reports Findings: None, with no inline comments or actionable summary details. All 24 review threads are resolved. The generic overview still says unspecified concerns remain, so this is not represented as formal approval.

Final rerun: 136 tests pass, typecheck passes, git diff --check passes, and the worktree is clean. Both current-head CI runs pass. Valid findings were fixed with reproduced regressions; three false positives (scored rename parsing, full item serialization, and .git target checking) were declined with executable evidence. PR remains unmerged. Runtime/store integration remains tracked in #6; ledger persistence (#2) is next in the approved order.

mchwang and others added 6 commits September 22, 2026 15:49
- DESIGN.md: tokens, type, layout, components (provenance gutter,
  file-change cards, logo), motion, and a decisions log.
- CLAUDE.md: point UI work at DESIGN.md.
- Design doc: screen specifications from the design review (D12-D28)
  and mark D22 done.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- schema/plan.schema.json: plan format v1, strict-mode compatible, used by
  claude --json-schema and codex exec --output-schema and for imports.
- schema/plan-edit.schema.json: typed plan-assistant suggestions.
- schema/examples/: #412 r3 plan (YAML) and a suggestion reply.
- docs/plan-format.md: plain-language guide, checks after import, versions.
- prompts/plan-author.md: one prompt for both agents.
- Design doc: rewrite "The plan format", add decision P1 and task T18,
  list the four new approved mockups.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ding

- Design doc: lock down agent containers (read-only root, tmpfs scratch,
  non-root, no capabilities, no host mounts, resource limits) with a
  start-up self-test; /work is the only lasting writable place. T1 verify
  steps extended. Fix the last "three checks" to four.
- plan-format.md: reject symlink components and .git targets in path and
  renamed_from; after each run, reject undeclared symlinks, symlinks that
  leave the repo, and any .git change; run codeboost's git with hooks off.
- Both schemas, the guide, and the prompt describe cmd as one program with
  literal arguments run without a shell. Example P2 command no longer
  contains a quoted pipe.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 23, 2026 00:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical Git alias execution risk and moderate attribution and choice-validation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Implements the first executable review-core slice: plan validation, safe suggestions, read-only Git history, ledger-backed attribution, and approval tracking.

Changes:

  • Adds plan import, schema validation, command parsing, and suggestions.
  • Adds linear Git history loading and change attribution.
  • Adds approval/choice staleness handling.
  • Adds tests, documentation, strict TypeScript configuration, dependencies, and CI.
File Changes Review notes
tsconfig.json Strict TypeScript configuration —
test/​plan.test.ts Plan validation and suggestion tests —
test/​history.test.ts Git attribution and approval tests —
README.md Library usage and limitations —
package.json Scripts and dependencies —
package-lock.json Locked dependency versions —
git/​history.ts Read-only Git history adapter Critical (1 vote): Local Git aliases can execute arbitrary shell commands; use sanitized configuration with aliases disabled for every subcommand.
docs/​implementation/​build-step-1.md Implementation decisions and validation —
core/​plan.ts Plan validation and suggestions —
core/​linking.ts Ledger-backed change attribution Moderate (2 votes): Scope text segments using the path actually changed, retaining both paths only for rename/file-card evidence.
Moderate (1 vote): Preserve replayed rename provenance when rename detection falls back to delete/add deltas.
Moderate (1 vote): Include moved owners when calculating replacement ownership so rename ancestry remains ambiguous where appropriate.
Moderate (1 vote): Replay path deltas from a pre-commit snapshot before applying staged updates to handle swaps and rotations correctly.
core/​approvals.ts Approval fingerprints and choices Moderate (1 vote): Validate that accept has a null item and assign has a non-null item; reject malformed or future actions rather than treating them as assignments.
.gitignore Generated-file exclusions —
.github/​workflows/​ci.yml Node CI workflow —

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread git/history.ts Outdated
Comment thread core/linking.ts Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 00:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in Git isolation, attribution, and approval handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve rename provenance through deletion

core/​linking.ts:86

Metadata provenance is only stored under the immediately previous path. For a history a.txt -> b.txt owned by P1 followed by deleting b.txt owned by P2, the final base-to-head diff is a deletion of a.txt; this lookup then reports the file-change card as P1, while the actual deletion (and its text removals) belongs to P2. Carry metadata provenance through rename path ancestry (and merge the delete owner) before classifying the final card; add a rename-then-delete regression.

Comment thread git/history.ts Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 01:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect repository isolation, history bounds, attribution, and approval state.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread git/history.ts Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 01:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved provenance, repository-isolation, and aggregate-memory findings block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (1)

Comment thread core/linking.ts Outdated
Comment thread git/history.ts Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 01:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Seven final review findings remain unresolved, including one critical aggregate-memory issue in git/history.ts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread git/history.ts
Copilot AI review requested due to automatic review settings September 23, 2026 01:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Six unresolved findings remain, including one critical repository object-storage isolation issue.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread git/history.ts Outdated
@mchwang
mchwang changed the base branch from design/evidence-desk to main September 23, 2026 05:37
Copilot AI review requested due to automatic review settings September 23, 2026 05:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved Git rename parsing, attribution, and target-validation issues remain, along with documentation nits.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 2 Low severity

Open (3)

Comment thread git/history.ts
Comment thread README.md
Comment thread README.md Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 05:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect approval staleness, repository safety, parsing, history attribution, and documentation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (3)

Comment thread core/approvals.ts
Comment thread core/plan.ts Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 06:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in plan validation, Git history handling, and ledger linking.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 6 High severity

Open (6)
Resolved since last review (2)

Comment thread core/linking.ts Outdated
Comment thread core/linking.ts Outdated
Comment thread core/plan.ts Outdated
Comment thread core/plan.ts
Comment thread core/plan.ts Outdated
Comment thread git/history.ts Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 06:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Four unresolved moderate findings remain in core/linking.ts, core/parse-v1.ts, core/plan.ts, and git/history.ts.

Review effort: Lite
Findings: None

Resolved since last review (6)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve metadata when deleted paths are recreated

core/​linking.ts:152

The global metadata map is overwritten when a path is deleted and later recreated (or when a new file reuses a previously deleted path). A final binary/mode/rename card for that path then loses the earlier deletion owner's metadata and can be shown only under the recreating commit, unlike the line-level attribution. Merge any existing path evidence into the value before storing it so reused paths retain all contributing owners.

@mchwang

mchwang commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the summary-only finding in review 5287702535. A binary delete/recreate regression first retained only P2; metadata updates now merge existing path evidence before storing and propagating it. Owned deletion plus recreation retains P1/P2 as Ambiguous, and foreign deletion remains Unplanned. Final suite: 134 tests passing; typecheck and git diff --check pass.

Copilot AI review requested due to automatic review settings September 23, 2026 06:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved deterministic approval serialization and Git graft-handling issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)

Comment thread git/history.ts Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 06:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Three unresolved moderate findings remain in core/plan.ts.

Review effort: Lite
Findings: None

Resolved since last review (1)

@mchwang
mchwang merged commit 550340a into main Sep 23, 2026
3 checks passed
@mchwang
mchwang deleted the codex/plan-format-and-linking branch September 23, 2026 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants